Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bundling changes/improvements #1831

Merged
merged 11 commits into from
Jan 14, 2019
Merged

Bundling changes/improvements #1831

merged 11 commits into from
Jan 14, 2019

Conversation

philipwalton
Copy link
Member

@philipwalton philipwalton commented Jan 11, 2019

As a first step toward the changes proposed in #1830, I've updated the workbox-routing and workbox-core packages to give you an idea of what I'm thinking. If no one has any serious objections, I'll go ahead and update the rest of the packages as well.

The primary things I've done here are:

  • Remove all default exports
  • Move default export methods into their own files (exported individually)
  • Removed _public.mjs, _default.mjs, and browser.mjs files
  • Removed the log level code
  • Updated the build script with the changes
  • Moved the skipWaiting() and clientClaim() methods from workbox-sw to workbox-core (see here for rationale)
  • Deprecated the strategy functions in favor of using strategy classes (similar to how we do plugins).

R: @jeffposnick @gauntface

@philipwalton philipwalton changed the title Exports update Bundling changes/improvements Jan 11, 2019
@philipwalton
Copy link
Member Author

I finished updating the rest of the packages, and I updated all the tests accordingly.

I know this is a lot of changes to review, but overall I think these changes make our package structure much simpler. Also, it looks like it reduced our overall code size by more than 1KB (gzipped) from the previous PR. We're now in the 8KB range! 🎉

Copy link
Contributor

@jeffposnick jeffposnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking workbox.strategies.<strategyName> deprecated in favor of new workbox.strategies.StrategyName() is also a pretty big change that got rolled this PR, which wasn't included in the description. It's worth calling that out because we're going to have to communicate that in the release notes and update a bunch fo documentation examples.

packages/workbox-core/index.mjs Outdated Show resolved Hide resolved
packages/workbox-precaching/utils/deleteOutdatedCaches.mjs Outdated Show resolved Hide resolved
packages/workbox-precaching/utils/precachePlugins.mjs Outdated Show resolved Hide resolved
@philipwalton
Copy link
Member Author

Marking workbox.strategies. deprecated in favor of new workbox.strategies.StrategyName() is also a pretty big change that got rolled this PR, which wasn't included in the description.

Ahh, sorry, I updated that in the proposal doc, but I forgot to also add it to the bullet points in this issue.

FWIW, I'm open to other options as well. Here are a few I think are find in a rough order of preference:

  1. Switch to using constructors (e.g. new CacheFirst()) everywhere (like we do with plugins), but continue to support the old strategy functions for one major version and log a deprecation warning (what this PR does).
  2. Keep the strategy functions and rename the strategy classes to something like CacheFirstStrategy (note we can't put them in a sub-folder because it would break the convention of keeping everything public in the top-level directory).
  3. Consider the strategy classes private and only expose the strategy functions. (Someone would still extend a strategy class, they'd just have to instantiate one first, which is a bit awkward).

@jeffposnick
Copy link
Contributor

I'm cool with moving to new workbox.strategies.StrategyName() in general. Having both ways of creating strategies has been a source of confusion in the past. My comment was mainly that we need to remember to call out that change, as it's going to be very visible to users.

const strategyString =
`workbox.strategies.${entry.handler}(${optionsString})`;
should also get updated to use the constructor syntax. This PR is already large enough, so that could be deferred until after this PR is merged.

@philipwalton
Copy link
Member Author

Ok, I can address that in a new PR. The rest of the stuff you mentioned has been updated.

@workbox-pr-bot
Copy link
Collaborator

PR-Bot Size Plugin

Changed File Sizes

File Before After Change GZipped
packages/workbox-background-sync/build/workbox-background-sync.prod.js 3.51 KB 3.59 KB +2% 1.56 KB
packages/workbox-broadcast-cache-update/build/workbox-broadcast-cache-update.prod.js 1.12 KB 1.88 KB +68% 940 B ☠️
packages/workbox-build/build/index.js 4.02 KB 3.64 KB -9% 1.36 KB
packages/workbox-cache-expiration/build/workbox-cache-expiration.prod.js 3.88 KB 3.15 KB -19% 1.28 KB 🎉
packages/workbox-cacheable-response/build/workbox-cacheable-response.prod.js 587 B 594 B +1% 350 B
packages/workbox-cli/build/app.js 6.76 KB 5.58 KB -17% 1.98 KB 🎉
packages/workbox-cli/build/bin.js 2.32 KB 1.16 KB -50% 580 B 🎉
packages/workbox-core/build/workbox-core.prod.js 7.47 KB 5.51 KB -26% 2.38 KB 🎉
packages/workbox-navigation-preload/build/workbox-navigation-preload.prod.js 660 B 667 B +1% 323 B
packages/workbox-precaching/build/workbox-precaching.prod.js 5.80 KB 4.20 KB -28% 1.68 KB 🎉
packages/workbox-range-requests/build/workbox-range-requests.prod.js 1.63 KB 1.53 KB -6% 764 B
packages/workbox-routing/build/workbox-routing.prod.js 2.87 KB 3.22 KB +12% 1.41 KB ☠️
packages/workbox-strategies/build/workbox-strategies.prod.js 5.09 KB 4.87 KB -4% 1.19 KB
packages/workbox-streams/build/workbox-streams.prod.js 1.57 KB 1.39 KB -12% 683 B 🎉
packages/workbox-sw/build/workbox-sw.js 1.50 KB 1.36 KB -9% 748 B
packages/workbox-webpack-plugin/build/generate-sw.js 8.04 KB 5.29 KB -34% 1.84 KB 🎉
packages/workbox-webpack-plugin/build/index.js 742 B 349 B -53% 255 B 🎉
packages/workbox-webpack-plugin/build/inject-manifest.js 10.30 KB 6.91 KB -33% 2.40 KB 🎉

New Files

File Size GZipped
packages/workbox-google-analytics/build/workbox-offline-ga.prod.js 1.74 KB 877 B

All File Sizes

View Table
File Before After Change GZipped
packages/workbox-background-sync/build/workbox-background-sync.prod.js 3.51 KB 3.59 KB +2% 1.56 KB
packages/workbox-broadcast-cache-update/build/workbox-broadcast-cache-update.prod.js 1.12 KB 1.88 KB +68% 940 B ☠️
packages/workbox-build/build/_types.js 41 B 41 B 0% 61 B
packages/workbox-build/build/index.js 4.02 KB 3.64 KB -9% 1.36 KB
packages/workbox-cache-expiration/build/workbox-cache-expiration.prod.js 3.88 KB 3.15 KB -19% 1.28 KB 🎉
packages/workbox-cacheable-response/build/workbox-cacheable-response.prod.js 587 B 594 B +1% 350 B
packages/workbox-cli/build/app.js 6.76 KB 5.58 KB -17% 1.98 KB 🎉
packages/workbox-cli/build/bin.js 2.32 KB 1.16 KB -50% 580 B 🎉
packages/workbox-core/build/workbox-core.prod.js 7.47 KB 5.51 KB -26% 2.38 KB 🎉
packages/workbox-google-analytics/build/workbox-offline-ga.prod.js 1.74 KB 877 B
packages/workbox-navigation-preload/build/workbox-navigation-preload.prod.js 660 B 667 B +1% 323 B
packages/workbox-precaching/build/workbox-precaching.prod.js 5.80 KB 4.20 KB -28% 1.68 KB 🎉
packages/workbox-range-requests/build/workbox-range-requests.prod.js 1.63 KB 1.53 KB -6% 764 B
packages/workbox-routing/build/workbox-routing.prod.js 2.87 KB 3.22 KB +12% 1.41 KB ☠️
packages/workbox-strategies/build/workbox-strategies.prod.js 5.09 KB 4.87 KB -4% 1.19 KB
packages/workbox-streams/build/workbox-streams.prod.js 1.57 KB 1.39 KB -12% 683 B 🎉
packages/workbox-sw/build/workbox-sw.js 1.50 KB 1.36 KB -9% 748 B
packages/workbox-webpack-plugin/build/generate-sw.js 8.04 KB 5.29 KB -34% 1.84 KB 🎉
packages/workbox-webpack-plugin/build/index.js 742 B 349 B -53% 255 B 🎉
packages/workbox-webpack-plugin/build/inject-manifest.js 10.30 KB 6.91 KB -33% 2.40 KB 🎉

Workbox Aggregate Size Plugin

8.83KB gzip'ed (59% of limit)
22.38KB uncompressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants